Skip to content

Two tests Ported from the LLVM Project to Translator#3342

Merged
MrSidims merged 1 commit intoKhronosGroup:mainfrom
SubashBoopathi:port_test3
Sep 29, 2025
Merged

Two tests Ported from the LLVM Project to Translator#3342
MrSidims merged 1 commit intoKhronosGroup:mainfrom
SubashBoopathi:port_test3

Conversation

@SubashBoopathi
Copy link
Contributor

Ported two tests from the LLVM project to the SPIRV-LLVM-Translator: icmp.ll validates integer comparison instructions for scalar and vector types, and fp-vector-intrinsics.ll verifies floating-point vector intrinsics, ensuring accurate SPIR-V translation.

@@ -0,0 +1,165 @@
; RUN: llvm-as %s -o %t.bc
Copy link
Contributor

@MrSidims MrSidims Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do remember this test was failing, when I first saw the PR. What has changed since then? Force-pushes are making it impossible for me to find out.

Copy link
Contributor Author

@SubashBoopathi SubashBoopathi Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test by removing some extra CHECK directives that are irrelevant and now passes all RUN commands. I’ll avoid force-pushes or add clearer notes next time to make changes easier to follow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly, that reverse translation was failing? I'm not saying, that you should fix this, but I'm trying to understand, whether we have a bug anywhere and we just had an opportunity to catch and file it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MrSidims The issue wasn’t related to reverse translation. Initially, I planned to include checks for some additional intrinsics, but since those intrinsics already had tests elsewhere, I removed their checks from the test file. Unfortunately, my initial removal wasn’t complete, which caused the failures you saw earlier. The fix was simply cleaning up those leftover, unnecessary CHECK directives of the removed intrinsics tests.

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me, @SubashBoopathi if it's possible, could you please check git reflog to get the removed CHECKs and submitting them as an issue to the translator, so we could verify, that them were wrong or there is something to fix in the translator?

@MrSidims MrSidims requested a review from svenvh September 29, 2025 10:31
@MrSidims MrSidims merged commit 095a8c2 into KhronosGroup:main Sep 29, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants